drivers: video: mt9m114: Add vertical and horizontal flip control #81830

jeronimoagullo · 2024-11-24T12:46:52Z

This pull request gives support to flip the camera mt9m114 images horizontally and vertically. For this aim, it allows to modify camera buffer reading mode.

This PR provides support for the use of video_set_ctrl function with the Camera class control IDs of VIDEO_CID_HFLIP and VIDEO_CID_VFLIP.

It was proposed by @ngphibang during a conversation. thanks!

ngphibang · 2024-11-24T15:01:06Z

@jeronimoagullo : Thank you for the commit !
Could you point out the platform you tested this control and some hflipped / vflipped images of it ?

jeronimoagullo · 2024-11-24T15:04:02Z

yes, @ngphibang I used the NXP imxRT1064 evaluation board. Besides, to check it out easily I modified the samples/drivers/video/tcpserversink sample to test it.

josuah

Thank you! Having the image in correct orientation is always needed...
I proposed some minor modification about error handling, but that is really not critical.

drivers/video/mt9m114.c

josuah · 2024-11-24T16:15:45Z

drivers/video/mt9m114.c

+	ret |= mt9m114_write_reg(dev, MT9M114_CAM_SENSOR_CTRL_READ_MODE, sizeof(reg),
+				&reg);


Same proposal as below to turn this into ret = and immediately checking the error, which is a bit more verbose but helps avoiding subtle bugs in applications. I.e. checking if some operation is not supported (no point in retrying), or just some failure on I/O (a ladybug accidentally shorted the I2C.SDA and GND lines, ladybug forgiven and new attempt issued).

jeronimoagullo · 2024-11-24T19:18:32Z

@josuah You are totally right! I was following the code style of the video drivers and I did't figure out the possibility of confusion due to error mess. I have modified it and push, please check it out, thanks!

josuah

Thank you for the change! It looks ready to land to me.
I was also using ret |= before Zephyr, as when the only error code is -1 this works.

kartben · 2024-11-24T20:36:36Z

drivers/video/mt9m114.c

+
+	switch (cid) {
+	case VIDEO_CID_HFLIP:
+		ret = mt9m114_set_horizontal_mirror(dev, (int)value);


s/mirror/flip/

I really like the vim format of your comment!

:)
Note that it of course doesn't apply if you end up refactoring the two functions into one

Hi @kartben , as you said, this doesn't apply anymore due to refactoring (implemented differently), could you resolve the Change Request to help moving this PR forwards ?

kartben · 2024-11-24T20:37:32Z

drivers/video/mt9m114.c

+	}
+
+	if (enable) {
+		reg |= BIT(0);


Avoid magic numbers and #define this properly

kartben · 2024-11-24T20:37:40Z

drivers/video/mt9m114.c

+	}
+
+	if (enable) {
+		reg |= BIT(1);


kartben · 2024-11-24T20:41:12Z

drivers/video/mt9m114.c

+	return mt9m114_write_reg(dev, MT9M114_CAM_SENSOR_CTRL_READ_MODE, sizeof(reg), &reg);
+}
+
+static int mt9m114_set_vertical_flip(const struct device *dev, int enable)


Unless I missed something the two functions are after all essentially duplicated? Maybe make it a common set_orientation function?

You are right, I used two different functions like other camera driver does, but in this case, the functions may be the same adding just the bit that you want to change. Thus, I will merge them into just one function

kartben · 2024-11-25T08:46:10Z

Note that this will have conflicts when #78802 gets merged

ngphibang · 2024-11-25T08:51:33Z

The VIDEO_CID_HFLIP/VIDEO_CID_VFLIP CID naming are not changed in #78802 so I think it will not have conflicts.

BTW, I needs to test this PR on RT1064 before giving some reviews.

jeronimoagullo · 2024-11-25T08:58:58Z

Note that this will have conflicts when #78802 gets merged

Are you sure? I have just added flip control. So, I use only from video-contro.h the VIDEO_CID_HFLIP and VIDEO_CID_VFLIP macros which are not modified in #78802. Thus, for the moment, both PRs shouldn't have conflicts, right?

kartben · 2024-11-25T09:10:56Z

Note that this will have conflicts when #78802 gets merged

Are you sure? I have just added flip control. So, I use only from video-contro.h the VIDEO_CID_HFLIP and VIDEO_CID_VFLIP macros which are not modified in #78802. Thus, for the moment, both PRs shouldn't have conflicts, right?

ah yes sorry :) I thought all the macros had been renamed, but turns out these ones didn't have a category prefix in the first place. Sorry!

ngphibang · 2024-11-25T12:35:14Z

drivers/video/mt9m114.c

@@ -53,6 +55,10 @@ LOG_MODULE_REGISTER(video_mt9m114, CONFIG_VIDEO_LOG_LEVEL);
 #define MT9M114_CAM_OUTPUT_FORMAT_FORMAT_YUV (0 << 8)
 #define MT9M114_CAM_OUTPUT_FORMAT_FORMAT_RGB (1 << 8)

+/* Camera control masks */
+#define MT9M114_CAM_CONTROL_HORIZ_FLIP_EN    BIT(0)


I suggest : MT9M114_CAM_SENSOR_CTRL_HORZ_FLIP_EN to be coherent with MT9M114_CAM_SENSOR_CTRL_READ_MODE and MT9M114_CAM_SENSOR_CTRL_VERT_FLIP_EN

ngphibang · 2024-11-25T12:35:18Z

drivers/video/mt9m114.c

@@ -53,6 +55,10 @@ LOG_MODULE_REGISTER(video_mt9m114, CONFIG_VIDEO_LOG_LEVEL);
 #define MT9M114_CAM_OUTPUT_FORMAT_FORMAT_YUV (0 << 8)
 #define MT9M114_CAM_OUTPUT_FORMAT_FORMAT_RGB (1 << 8)

+/* Camera control masks */
+#define MT9M114_CAM_CONTROL_HORIZ_FLIP_EN    BIT(0)
+#define MT9M114_CAM_CONTROL_VERT_FLIP_EN     BIT(1)


I suggest : MT9M114_CAM_SENSOR_CTRL_VERT_FLIP_EN to be coherent with MT9M114_CAM_SENSOR_CTRL_READ_MODE

ngphibang · 2024-11-25T12:36:51Z

drivers/video/mt9m114.c

@@ -381,6 +387,26 @@ static int mt9m114_set_output_format(const struct device *dev, int pixel_format)
 	return ret;
 }

+static int mt9m114_set_orientation(const struct device *dev, int enable, uint8_t mask)


I would drop this function. Just use mt9m114_modify_reg() directly as suggested in comments below

Now that mt9m114_modify_reg() is used everywhere, this function can be removed.
Thanks for the modifications!

ngphibang · 2024-11-25T12:40:59Z

drivers/video/mt9m114.c

+
+	switch (cid) {
+	case VIDEO_CID_HFLIP:
+		ret = mt9m114_set_orientation(dev, (int)value, MT9M114_CAM_CONTROL_HORIZ_FLIP_EN);


mt9m114_modify_reg(dev, MT9M114_CAM_SENSOR_CTRL_READ_MODE, MT9M114_CAM_SENSOR_CTRL_HORZ_FLIP_EN, (int)value ? MT9M114_CAM_SENSOR_CTRL_HORZ_FLIP_EN : 0);

I see your point, but it is not feasible right now due to the definition of mt9m114_modify_reg function which works only with 8 bit registers. In this case, the MT9M114_CAM_SENSOR_CTRL_READ_MODE register is 16 bit.

I see two alternatives:

rewrite function to work with 8, 16 and 32 bit registers

write a function for each register size

Then, it would be feasible

mt9m114_modify_reg() actually works with 16 bits registers. It 's just a combination of mt9m114_read_reg() and mt9m114_write_reg():

mt9m114_modify_reg(const struct device *dev, const uint16_t addr, const uint8_t mask, const uint8_t val)

Ah, sorry, I just double checked. Indeed, inside mt9m114_modify_reg , it calls read/write on 8-bit regs only:

uint8_t newVal; int ret = mt9m114_read_reg(dev, addr, sizeof(oldVal), &oldVal);

So if possible, could you also modify mt9m114_modify_reg so that it supports both 8 bits and 16 bits (and 32 bits) ? then mt9m114_modify_reg is only used here and on MT9M114_RST_AND_MISC_CONTROL in software reset so the changes will not be much ?

Maybe it becomes interesting to have a CCI (Camera Common Interface) library that has read/write/modify built-in for various sizes.

I think that it is beyond the scope of this pull request, but it is something interested. However, right now we are using only an i2c bus. Thus, it would be interesting in case that we can configure cameras with other buses like SPI, otherwise, I don't see the point.

Yes absolutely, not related to this PR! I am thinking in the background: someone with a camera sensor knows the I2C address to toggle a feature and wants it upstream: how to make it just a 1-line diff to facilitate contribution.

I agree though: good not to introduce unneeded abstraction.
The way it is done here is probably the best solution.

@josuah I think adding CCI would be helpful. But even with CCI, in Linux, camera drivers still uses different i2c APIs to read / write registers, so it will not make it a "standard way" for every drivers

FYI, this is now proposed here to save effort while writing/modifying new drivers:

drivers: video: common: introduce CCI utilities #87935

ngphibang · 2024-11-25T12:42:29Z

drivers/video/mt9m114.c

+		ret = mt9m114_set_orientation(dev, (int)value, MT9M114_CAM_CONTROL_HORIZ_FLIP_EN);
+		break;
+	case VIDEO_CID_VFLIP:
+		ret = mt9m114_set_orientation(dev, (int)value, MT9M114_CAM_CONTROL_VERT_FLIP_EN);


mt9m114_modify_reg(dev, MT9M114_CAM_SENSOR_CTRL_READ_MODE, MT9M114_CAM_SENSOR_CTRL_VERT_FLIP_EN, (int)value ? MT9M114_CAM_SENSOR_CTRL_VERT_FLIP_EN : 0);

josuah

One more minor step...

This fixes mt9m114_modify_reg() which did not accept a size parameter.

josuah · 2024-11-25T22:00:59Z

drivers/video/mt9m114.c

@@ -381,6 +387,26 @@ static int mt9m114_set_output_format(const struct device *dev, int pixel_format)
 	return ret;
 }

+static int mt9m114_set_orientation(const struct device *dev, int enable, uint8_t mask)


Now that mt9m114_modify_reg() is used everywhere, this function can be removed.
Thanks for the modifications!

jeronimoagullo · 2024-11-26T08:23:54Z

@josuah check it out now, I have removed the mt9m114_set_orientation function and did a git rebase to be inline with main branch. I have tested it in the imxRT1064 successfully :)

ngphibang

@jeronimoagullo Just a few things left then it will be OK. Thanks for the changes !

drivers/video/mt9m114.c

ngphibang · 2024-11-26T08:47:51Z

drivers/video/mt9m114.c

@@ -261,20 +267,21 @@ static int mt9m114_read_reg(const struct device *dev, uint16_t reg_addr, uint8_t
 	return 0;
 }

-static int mt9m114_modify_reg(const struct device *dev, const uint16_t addr, const uint8_t mask,
-			      const uint8_t val)
+static int mt9m114_modify_reg(const struct device *dev, const uint16_t addr,


I think the extension of mt9m114_modify_reg() should go into a separate commit.

In that case, I should create a separate commit for it, merge it and then carry on with this PR. Do you agree?

Yes, just split the current commit into 2 commits then force-push in this same PR.

ngphibang · 2024-11-26T08:48:44Z

drivers/video/mt9m114.c

@@ -297,15 +304,15 @@ static int mt9m114_write_all(const struct device *dev, struct mt9m114_reg *reg)

 static int mt9m114_software_reset(const struct device *dev)
 {
-	int ret = mt9m114_modify_reg(dev, MT9M114_RST_AND_MISC_CONTROL, 0x01, 0x01);
+	int ret = mt9m114_modify_reg(dev, MT9M114_RST_AND_MISC_CONTROL, 1, 0x01, 0x01);


MT9M114_RST_AND_MISC_CONTROL seems a 16-bit register ?

You are rigth! I have checkout the datasheet and it is 16 bit. I got confused since it was being modified by the 8-bit function like it was an 8-bit register. I will update it correctly.

jeronimoagullo · 2024-11-29T06:32:11Z

Hi! @kartben and @loicpoulain do you agree with the final version of the Pull Request? Thanks!

loicpoulain

LGTM

ngphibang · 2024-12-06T07:28:27Z

@jeronimoagullo Please rebase your branch on the lastest main to resolve the conflict.

ngphibang · 2024-12-09T09:40:22Z

@jeronimoagullo hey, can you do a (simple) git rebase to resolve the conflict ? It's because the #82301 was merged.

…ngth Update mt9m114_modify_reg to support 8, 16 and 32 bits registers Signed-off-by: Jeronimo Agullo <jeronimoagullo97@gmail.com>

Add set_ctrl function API for vertical and horizontal flip control modifying the camera read mode Signed-off-by: Jeronimo Agullo <jeronimoagullo97@gmail.com>

ngphibang · 2024-12-10T08:50:58Z

@kartben Could you revisit this ? thanks !

kartben · 2024-12-12T09:25:54Z

@kartben Could you revisit this ? thanks !

Of course, sorry!

zephyrbot added the area: Video Video subsystem label Nov 24, 2024

zephyrbot requested review from josuah, loicpoulain and ngphibang November 24, 2024 12:48

josuah requested changes Nov 24, 2024

View reviewed changes

josuah added area: Drivers area: camera labels Nov 24, 2024

jeronimoagullo force-pushed the mt9m114_flip branch from 0f76f83 to c72075a Compare November 24, 2024 19:16

jeronimoagullo requested a review from josuah November 24, 2024 19:18

jeronimoagullo force-pushed the mt9m114_flip branch from c72075a to 071bdde Compare November 24, 2024 19:38

josuah previously approved these changes Nov 24, 2024

View reviewed changes

kartben requested changes Nov 24, 2024

View reviewed changes

jeronimoagullo dismissed josuah’s stale review via 1be6b27 November 25, 2024 07:43

jeronimoagullo force-pushed the mt9m114_flip branch from 071bdde to 1be6b27 Compare November 25, 2024 07:43

jeronimoagullo requested a review from kartben November 25, 2024 07:55

ngphibang requested changes Nov 25, 2024

View reviewed changes

jeronimoagullo force-pushed the mt9m114_flip branch 3 times, most recently from 217c401 to c7568e2 Compare November 25, 2024 18:01

josuah requested changes Nov 25, 2024

View reviewed changes

jeronimoagullo force-pushed the mt9m114_flip branch 2 times, most recently from fd175f5 to a5e2c0d Compare November 26, 2024 08:22

jeronimoagullo requested a review from ngphibang November 26, 2024 08:24

loicpoulain previously approved these changes Nov 26, 2024

View reviewed changes

ngphibang reviewed Nov 26, 2024

View reviewed changes

jeronimoagullo dismissed loicpoulain’s stale review via 9cb9818 November 26, 2024 09:43

jeronimoagullo force-pushed the mt9m114_flip branch from a5e2c0d to 9cb9818 Compare November 26, 2024 09:43

jeronimoagullo requested a review from ngphibang November 26, 2024 09:44

ngphibang previously approved these changes Nov 26, 2024

View reviewed changes

jeronimoagullo requested a review from loicpoulain November 26, 2024 10:13

josuah previously approved these changes Nov 26, 2024

View reviewed changes

ngphibang self-assigned this Nov 27, 2024

loicpoulain previously approved these changes Dec 1, 2024

View reviewed changes

ngphibang mentioned this pull request Dec 6, 2024

video: samples: i.MX RT10xx: Do not use PxP to flip images #82672

Merged

jeronimoagullo added 2 commits December 10, 2024 08:27

drivers: video: mt9m114: update mt9m114_modify_reg to any register le…

7f5b437

…ngth Update mt9m114_modify_reg to support 8, 16 and 32 bits registers Signed-off-by: Jeronimo Agullo <jeronimoagullo97@gmail.com>

drivers: video: mt9m114: Add vertical and horizontal flip control

5578c9f

Add set_ctrl function API for vertical and horizontal flip control modifying the camera read mode Signed-off-by: Jeronimo Agullo <jeronimoagullo97@gmail.com>

jeronimoagullo dismissed stale reviews from loicpoulain, josuah, and ngphibang via 5578c9f December 10, 2024 07:29

jeronimoagullo force-pushed the mt9m114_flip branch from 9cb9818 to 5578c9f Compare December 10, 2024 07:29

ngphibang approved these changes Dec 10, 2024

View reviewed changes

ngphibang requested a review from josuah December 10, 2024 08:47

ngphibang requested a review from loicpoulain December 10, 2024 08:51

josuah approved these changes Dec 10, 2024

View reviewed changes

kartben approved these changes Dec 12, 2024

View reviewed changes

kartben merged commit b69004c into zephyrproject-rtos:main Dec 12, 2024
24 checks passed

josuah mentioned this pull request Jan 21, 2025

drivers: video: ov7670: Implement missing video API functions, controls and update init regs. #84229

Merged

		ret \|= mt9m114_write_reg(dev, MT9M114_CAM_SENSOR_CTRL_READ_MODE, sizeof(reg),
		&reg);

drivers: video: mt9m114: Add vertical and horizontal flip control #81830

drivers: video: mt9m114: Add vertical and horizontal flip control #81830

Uh oh!

Conversation

jeronimoagullo commented Nov 24, 2024

Uh oh!

ngphibang commented Nov 24, 2024

Uh oh!

jeronimoagullo commented Nov 24, 2024

Uh oh!

josuah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

josuah Nov 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeronimoagullo commented Nov 24, 2024

Uh oh!

josuah left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kartben commented Nov 25, 2024

Uh oh!

ngphibang commented Nov 25, 2024

Uh oh!

jeronimoagullo commented Nov 25, 2024

Uh oh!

kartben commented Nov 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngphibang Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngphibang Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngphibang Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josuah Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josuah Nov 24, 2024 •

edited

Loading

josuah left a comment •

edited

Loading

ngphibang Nov 25, 2024 •

edited

Loading

ngphibang Nov 25, 2024 •

edited

Loading

ngphibang Nov 25, 2024 •

edited

Loading

josuah Nov 25, 2024 •

edited

Loading

josuah Apr 1, 2025 •

edited

Loading

ngphibang Nov 25, 2024 •

edited

Loading

josuah left a comment •

edited

Loading

jeronimoagullo commented Nov 26, 2024 •

edited

Loading